feat: Component best practices- async flags check #73

d3xter666 · 2024-04-16T09:08:03Z

JIRA: CPOUI5FOUNDATION-792

src/linter/ui5Types/SourceFileLinter.ts

test/fixtures/linter/rules/BestPractices/Negative_1/Component.js

src/linter/ui5Types/SourceFileLinter.ts

test/lib/linter/rules/snapshots/CSPCompliance.ts.md

test/lib/linter/rules/snapshots/BestPractices.ts.md

src/linter/ui5Types/BestPractices.ts

src/linter/ui5Types/SourceFileLinter.ts

src/linter/ui5Types/BestPractices.ts

matz3

I don't see test cases for the other common base component classes in UI5:

sap/ui/generic/app/AppComponent
sap/suite/ui/generic/template/lib/AppComponent
sap/fe/core/AppComponent

d3xter666 · 2024-04-26T10:17:57Z

I don't see test cases for the other common base component classes in UI5:

sap/ui/generic/app/AppComponent
sap/suite/ui/generic/template/lib/AppComponent
sap/fe/core/AppComponent

These are all extending the sap/ui/core/UIComponent, so the general solution would handle them with the help of UI5 Types. Further more, if some of those classes starts implementing the async interface, that would also be matched :

ui5-linter/src/linter/ui5Types/BestPractices.ts

Line 40 in 4c4178e

    
           const uiComponentImportVar = moduleImports.reduce((varName: string, importClause: ts.Node) => {

This change updates `@sapui5/types` to 1.120.13 which is vital for #73

Snapshots reflect unexpected behavior. Negative tests should have no findings while positive tests should show findings.

…string extraction

…terface flag Since the async interface is either used or not, the information whether it is "not set" is redundant and the "parent property not set" information is not applicable.

Use enum value for implicit searching

Document test fixtures

RandomByte · 2024-05-21T09:56:04Z

src/linter/linter.ts

 	if (pathsToLint?.length) {
 		const absoluteFilePaths = resolveFilePaths(rootDir, pathsToLint);
 		resolvedFilePaths = transformFilePathsToVirtualPaths(
 			absoluteFilePaths, rootDir, "/", rootDir);
+		// Extract the (virtual) path from the filename
+		virBasePath = resolvedFilePaths[0].split("/").slice(0, -1).join("/");


What was the reason for adae2ac ?

I'm wondering whether this approach still works if the first path in the array is deeply nested. For example /resources/my/app/controller/MyController.js would lead to virtual base path /resources/my/app/controller/. Which does not match for other paths like /resources/my/app/view/MyView.view.xml

The reason for that change and the related ones is that for the test cases it used to read files one by one and didn't have the context for the parent components, for example.

The problem lies in the fact that without a /resource prefix, the files were not found. To be honest, I'm not sure for the core reason behnid this behaviour, but I guess it might be something with the Reader

Hmmm, you're right about the deeply nested components 🤔

But, how to extract the namespace without breaking the signature of the function?
Maybe, reuse the logic from lintProject(), but then we need a project, not a single file...

I reverted the commit and all tests are still green 😅

Unfortunately, it’s not just that change 😅
I have tried several things to provide a better handling of those resources.

Maybe, directly comparing/reseting the file with the main branch would be easier 😅

Hm, not sure I understand the implications...

I just pushed the revert which restores the behavior I think is better (and should work for deep paths). Let's discuss it based on that 👍

Please take a look here: src/linter/linter.ts

This is the original change compared to the main branch.
The change is working pretty well, except that it cannot handle /test-resources/ and that's why I needed to find a better way of doing it.
Please take a look at the conversation we had with @flovogt on that matter here: https://github.com/SAP/ui5-linter/pull/73/files#r1601613818

But, if we take a look at the implementation of the lintProject() method, it again relies only on the /resources path, ignoring the /test-resources.

If this is ok, then we can leave it as it is now :)

Got it, thanks. Since the lintFile API is currently only used in our tests, I think we can safely go with the current state and ignore the test-resources case for now 👍

src/linter/ui5Types/SourceFileLinter.ts

src/linter/ui5Types/bestPractices.ts

RandomByte · 2024-05-28T13:24:03Z

I'm fine with this PR and once #73 (comment) is clarified we can merge it.

As a follow-up I would like us to extract the texts similar to https://github.com/SAP/ui5-linter/pull/112/files#diff-9d3ca30e9cd9e1355253f085c67fd2abafe4dfff649602cacf8b3ddef81db990 and hand them over to UA for review.

This reverts commit adae2ac.

RandomByte · 2024-05-28T20:42:01Z

GitHub requires an additional review from someone other than me 😅 @flovogt

A CallExpression on a CallExpression wasn't handled yet and caused a fatal error. This commit also adds a test case for another issue that was already fixed via #73 (SuperKeyword). Fixes: #246

d3xter666 marked this pull request as draft April 16, 2024 09:08

matz3 changed the title ~~feat: Component best practices~~ feat: Detect Component best practices Apr 17, 2024

matz3 mentioned this pull request Apr 17, 2024

Roadmap: v1.0.0 #21

Closed

27 tasks

d3xter666 force-pushed the component-best-practices branch from f14afdd to ea65aa3 Compare April 18, 2024 13:48

d3xter666 requested a review from a team April 19, 2024 11:18

d3xter666 marked this pull request as ready for review April 19, 2024 11:18

matz3 reviewed Apr 19, 2024

View reviewed changes

src/linter/ui5Types/SourceFileLinter.ts Outdated Show resolved Hide resolved

src/linter/ui5Types/SourceFileLinter.ts Outdated Show resolved Hide resolved

matz3 reviewed Apr 19, 2024

View reviewed changes

test/fixtures/linter/rules/BestPractices/Negative_1/Component.js Outdated Show resolved Hide resolved

matz3 reviewed Apr 19, 2024

View reviewed changes

src/linter/ui5Types/SourceFileLinter.ts Outdated Show resolved Hide resolved

d3xter666 marked this pull request as draft April 24, 2024 06:43

d3xter666 marked this pull request as ready for review April 25, 2024 13:51

d3xter666 requested review from matz3 and a team April 25, 2024 13:51

matz3 reviewed Apr 26, 2024

View reviewed changes

d3xter666 force-pushed the component-best-practices branch from 66c09e8 to d60a47e Compare May 7, 2024 13:14

d3xter666 requested review from a team and matz3 May 8, 2024 05:49

d3xter666 mentioned this pull request May 13, 2024

deps: Bump @sapui5/types to 1.120.13 #116

Merged

d3xter666 added a commit that referenced this pull request May 14, 2024

deps: Bump @sapui5/types to 1.120.13 (#116)

6e0744e

This change updates `@sapui5/types` to 1.120.13 which is vital for #73

d3xter666 added 9 commits May 14, 2024 10:33

feat: Analyze async flags

e2bab81

feat: Implement manifest checks

46b4622

feat: Extend linting further

6e32f58

fix: JSON "parse" improvement

cda4880

feat: Handle variations for error messaging

c980c07

feat: Checks for inline manifest

7d5711d

fix: Tests

4e8d607

fix: Merge conflicts

3280f54

fix: Reset Component.js

a3899a0

RandomByte and others added 6 commits May 28, 2024 11:34

test(AsyncComponentFlags): Add more test cases

d2eabaa

Snapshots reflect unexpected behavior. Negative tests should have no findings while positive tests should show findings.

refactor(asyncComponentFlags): Remove usage of internal API, improve …

e7a8955

…string extraction

refactor(asyncComponentFlags): Rename types, use boolean for async in…

608dd67

…terface flag Since the async interface is either used or not, the information whether it is "not set" is redundant and the "parent property not set" information is not applicable.

refactor(asyncComponentFlags): Simplify async flag merge logic

a718397

Use enum value for implicit searching

test(AsyncComponentFlags): Move parent components into subdirs

992dee4

Document test fixtures

fix: Skip depcheck for fake modules in testing

f4be3cb

d3xter666 changed the title ~~feat: Detect Component best practices~~ feat: Component best practices: async flags check May 28, 2024

d3xter666 changed the title ~~feat: Component best practices: async flags check~~ feat: Component best practices- async flags check May 28, 2024

d3xter666 requested review from RandomByte, flovogt and matz3 May 28, 2024 11:59

refactor: Improve file path handling

85fda50

RandomByte reviewed May 28, 2024

View reviewed changes

RandomByte added 2 commits May 28, 2024 15:14

refactor: Update message texts

7790b4a

refactor: Fix message text

f136e64

RandomByte force-pushed the component-best-practices branch from 108c6a2 to f136e64 Compare May 28, 2024 13:22

refactor: Revert "refactor: Extract virBasePath from filePaths"

8e42467

This reverts commit adae2ac.

RandomByte force-pushed the component-best-practices branch from 8e42467 to f136e64 Compare May 28, 2024 20:28

RandomByte approved these changes May 28, 2024

View reviewed changes

flovogt approved these changes May 29, 2024

View reviewed changes

d3xter666 merged commit 1c58105 into main May 29, 2024
29 checks passed

d3xter666 deleted the component-best-practices branch May 29, 2024 07:31

openui5bot mentioned this pull request May 26, 2024

release: v0.2.3 #119

Merged

This was referenced Aug 9, 2024

fix: Unhandled CallExpression expression syntax: CallExpression #255

Merged

UI5 Linter fails to determine context of a call expression in common scenarios #246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Component best practices- async flags check #73

feat: Component best practices- async flags check #73

d3xter666 commented Apr 16, 2024

matz3 left a comment

d3xter666 commented Apr 26, 2024 •

edited

Loading

RandomByte May 21, 2024

d3xter666 May 28, 2024

d3xter666 May 28, 2024

RandomByte May 28, 2024

d3xter666 May 28, 2024 •

edited

Loading

RandomByte May 28, 2024

d3xter666 May 28, 2024 •

edited

Loading

RandomByte May 28, 2024

RandomByte commented May 28, 2024

RandomByte commented May 28, 2024

feat: Component best practices- async flags check #73

feat: Component best practices- async flags check #73

Conversation

d3xter666 commented Apr 16, 2024

matz3 left a comment

Choose a reason for hiding this comment

d3xter666 commented Apr 26, 2024 • edited Loading

RandomByte May 21, 2024

Choose a reason for hiding this comment

d3xter666 May 28, 2024

Choose a reason for hiding this comment

d3xter666 May 28, 2024

Choose a reason for hiding this comment

RandomByte May 28, 2024

Choose a reason for hiding this comment

d3xter666 May 28, 2024 • edited Loading

Choose a reason for hiding this comment

RandomByte May 28, 2024

Choose a reason for hiding this comment

d3xter666 May 28, 2024 • edited Loading

Choose a reason for hiding this comment

RandomByte May 28, 2024

Choose a reason for hiding this comment

RandomByte commented May 28, 2024

RandomByte commented May 28, 2024

d3xter666 commented Apr 26, 2024 •

edited

Loading

d3xter666 May 28, 2024 •

edited

Loading

d3xter666 May 28, 2024 •

edited

Loading